-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed bug limit push down of index scan. #2905
fixed bug limit push down of index scan. #2905
Conversation
7138b6a
to
1b1848e
Compare
src/storage/exec/IndexEdgeNode.h
Outdated
@@ -66,6 +62,7 @@ class IndexEdgeNode final : public RelNode<T> { | |||
edges.emplace_back(std::move(edge)); | |||
iter->next(); | |||
} | |||
int64_t count = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why count
is initialized with 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
count
is initialized with 1?
limit has three types of boundary value:
limit == -1, This means no limits and is a default value.
limit == 0, This means don't need to return any data. this one don't need to any data scan. handler in LookupProcessor.cpp:26
limit > 0, true used for limit. the count number begin with 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean you should initialize with 0? See line 82 limit_ > 0 && ++count > limit_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks ok
Because here put the data in first, and then judge
If count
is initialized to 0, the code on line 82 is as follows:
if (limit_> 0 && ++count >= limit_) {
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I prefer to initialize count
to 0, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I prefer to initialize
count
to 0, wdyt?
Aha, the point of ambiguity is here, you are all right. the code readability is a bit low.
Changed the init count to 0.
Close #2583 |
auto indexScan = std::make_unique<IndexScanNode<IndexID>>( | ||
context_.get(), indexId, std::move(colHints), limit_); | ||
auto indexScan = | ||
std::make_unique<IndexScanNode<IndexID>>(context_.get(), indexId, std::move(colHints)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this plan need limit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this plan need
limit
?
if FilterNode exists. will check the limit in FilterNode. skip the IndexScanNode.
1b1848e
to
23bf1ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
related PR #2796
The logic of the previous PR is simple, handles limits during the index scan phase. Scanning filter, data validity, etc., are not considered.
for this PR , The items below will be covered by:
Temporarily unsupported
Limit + Paging Scan
is integrated